Add memory usage statistics for slabs and allocation classes #91
Conversation
igchor
force-pushed
the
more_stats
branch
2 times, most recently
from
c880d56 to
891ad6b
Compare
|
Hi @igchor, most of the metrics have the name and value separated by a single colon like: For the line below, I'm thinking how we would parse this output into 'name, value' for prometheus If we want to follow the same format as other metrics maybe we could use this format: Does it make sense to change the stats output to use this format or something like this? Our existing scripts will automatically collect new metrics output in this format. |
|
should this name have approx term in it as well ? |
|
Do you want to use memory_order_seq_cst order here for atomic store ? Otherwise store() is needed if you want to use something like memory_order_release order. |
|
same as above comment about memory order if using assignment operator instead of atomic store(). |
|
Cosmetic review : I suggest using curly braces even if there is one line after if statement. |
|
am not sure this line will pass the clang-format check. the character count is high. |
There was a problem hiding this comment.
Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @byrnedj and @igchor)
There was a problem hiding this comment.
@raema Ok, I changed it to how you suggested. Please let me now if this is OK.
Reviewable status: 7 of 15 files reviewed, 5 unresolved discussions (waiting on @byrnedj and @guptask)
cachelib/allocator/CacheAllocator-inl.h line 2510 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
should this name have approx term in it as well ?
Done.
cachelib/allocator/memory/AllocationClass.cpp line 54 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
Do you want to use memory_order_seq_cst order here for atomic store ? Otherwise store() is needed if you want to use something like memory_order_release order.
It's just for readability, the order doesn't matter here since it's ctor.
cachelib/allocator/memory/AllocationClass.cpp line 126 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
same as above comment about memory order if using assignment operator instead of atomic store().
Same as above.
cachelib/allocator/memory/AllocationClass.cpp line 741 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
Cosmetic review : I suggest using curly braces even if there is one line after if statement.
Done.
cachelib/allocator/tests/CacheBaseTest.cpp line 36 at r1 (raw file):
Previously, guptask (Sounak Gupta) wrote…
am not sure why this line passed the clang-format check ? the character count is high.
Done.
This output looks good. Thanks! |
| } | ||
| } | ||
|
|
||
| auto maxBatch = *std::max_element(batches.begin(), batches.end()); |
There was a problem hiding this comment.
will segfault if batches.size() == 0
There was a problem hiding this comment.
Good catch, I commited this file by mistake.
Reviewable status: 7 of 15 files reviewed, 6 unresolved discussions (waiting on @byrnedj and @guptask)
cachelib/allocator/PromotionStrategy.h line 52 at r2 (raw file):
Previously, byrnedj (Daniel Byrne) wrote…
will segfault if batches.size() == 0
Done.
There was a problem hiding this comment.
Reviewable status: 7 of 15 files reviewed, 5 unresolved discussions (waiting on @byrnedj and @guptask)
Track latency of per item eviction/promotion between memory tiers
Printing those stats can be enabled by passing
-report_memory_usage_statsto cachebenchSample output:
This change is